Enhancement/11092 refactor reporting key events datapoints by abdelmalekkkkk · Pull Request #12391 · google/site-kit-wp

abdelmalekkkkk · 2026-03-26T14:03:25Z

Summary

Addresses issue:

Refactor Analytics 4 Reporting and Key Events Datapoints #11092

Relevant technical choices

PR Author Checklist

My code is tested and passes existing unit tests.
My code has an appropriate set of unit tests which all pass.
My code is backward-compatible with WordPress 5.2 and PHP 7.4.
My code follows the WordPress coding standards.
My code has proper inline documentation.
I have added a QA Brief on the issue linked above.
I have signed the Contributor License Agreement (see https://cla.developers.google.com/).

Do not alter or remove anything below. The following sections will be managed by moderators only.

Code Reviewer Checklist

Run the code.
Ensure the acceptance criteria are satisfied.
Reassess the implementation with the IB.
Ensure no unrelated changes are included.
Ensure CI checks pass.
Check Storybook where applicable.
Ensure there is a QA Brief.
Ensure there are no unexpected significant changes to file sizes.

Merge Reviewer Checklist

Ensure the PR has the correct target branch.
Double-check that the PR is okay to be merged.
Ensure the corresponding issue has a ZenHub release assigned.
Add a changelog message to the issue.

github-actions · 2026-03-26T14:08:20Z

🤖 This comment is automatically updated by CI workflows. Each section is managed independently.

📦 Build files for 5ab1bf9:

Download development build
Download production build

🎭 Playwright reports for 5ab1bf9:

hussain-t

Thanks, @abdelmalekkkkk! Could you please resolve the conflicts with the latest develop and send it back?

abdelmalekkkkk · 2026-04-06T16:02:47Z

@hussain-t Done! Back to you.

hussain-t

Thanks, @abdelmalekkkkk! Overall, the changes look good. However, I've left a concern and a few minor comments. Please take a look.

hussain-t · 2026-04-07T07:09:40Z

tests/phpunit/integration/Modules/Analytics_4/Datapoints/Get_ReportTest.php

+	 *
+	 * @var Analytics_4_Report_Response
+	 */
+	private $reportResponse;


Let's use snake_case as per WP standards.

Suggested change

private $reportResponse;

private $report_response;

hussain-t · 2026-04-07T07:11:09Z

tests/phpunit/integration/Modules/Analytics_4/Datapoints/Get_Has_Property_AccessTest.php

+		}
+	}
+
+


Please remove the extra blank line.

Suggested change

hussain-t · 2026-04-07T07:11:42Z

tests/phpunit/integration/Modules/Analytics_4/Datapoints/Get_Has_Property_AccessTest.php

+	 */
+	private $analytics;
+
+


Please remove the extra blank line.

Suggested change

hussain-t · 2026-04-07T07:13:10Z

tests/phpunit/integration/Modules/Analytics_4/Datapoints/Get_ReportTest.php

+				'service'           => function () use ( $service ) {
+					return $service;
+				},
+				'shareable'         => true,


Get_Report extends Shareable_Datapoint which unconditionally returns true from is_shareable(). Passing 'shareable' => true in the definition is redundant.

Suggested change

'shareable' => true,

hussain-t · 2026-04-07T07:14:23Z

tests/phpunit/integration/Modules/Analytics_4/Datapoints/Get_Key_EventsTest.php

+
+use Google\Site_Kit\Core\REST_API\Data_Request;
+use Google\Site_Kit\Modules\Analytics_4\Datapoints\Get_Key_Events;
+use Google\Site_Kit\Modules\Analytics_4\Report\Response as ReportResponse;


Let's remove this unused import.

Suggested change

use Google\Site_Kit\Modules\Analytics_4\Report\Response as ReportResponse;

hussain-t · 2026-04-07T07:17:48Z

includes/Modules/Analytics_4.php

+use Google\Site_Kit\Modules\Analytics_4\Datapoints\Get_Report;
+use Google\Site_Kit\Modules\Analytics_4\Datapoints\Get_Key_Events;
+use Google\Site_Kit\Modules\Analytics_4\Datapoints\Get_Has_Property_Access;


A nitpicky one, however, while we are at it, can you reorder these imports and group them with other Get_* imports?

hussain-t · 2026-04-07T07:32:24Z

includes/Core/Modules/Module.php

 	protected function is_shared_data_request( Data_Request $data ) {
-		$datapoint    = $this->get_datapoint_definition( "{$data->method}:{$data->datapoint}" );
-		$oauth_client = $this->get_oauth_client_for_datapoint( $datapoint );
+		$datapoint = $this->get_datapoint_definition( "{$data->method}:{$data->datapoint}" );


The refactored is_shared_data_request() no longer accounts for the scope validation fallback in get_oauth_client_for_datapoint().

Previously, if all sharing conditions were met but the owner's scopes failed validation, get_oauth_client_for_datapoint() would fall back to the default client, and is_shared_data_request() would return false (since the clients matched). Now it returns true regardless of scope validation.

This means shared-data restrictions (e.g. date range limits in GET:batch-report, metric/dimension validation in AdSense) could be applied to requests that are actually using the current user's own credentials — potentially restricting data the user should have full access to.

Should this method preserve the old scope-aware behavior?

Thanks for pointing that out. I thought checking the clients was just a quick way to determine if the request was shared. I reverted some of the changes and only left the new is_shared_datapoint_request. Please take another look.

abdelmalekkkkk · 2026-04-07T12:01:30Z

Thanks @hussain-t. I addressed all your comments. Please take another look.

hussain-t requested changes Apr 6, 2026

View reviewed changes

abdelmalekkkkk force-pushed the enhancement/11092-refactor-reporting-key-events-datapoints branch from 99e0f3f to bf546b0 Compare April 6, 2026 15:10

hussain-t requested changes Apr 7, 2026

View reviewed changes

abdelmalekkkkk added 7 commits April 7, 2026 12:31

Extract get report datapoint.

c14b6df

Add get report datapoint tests.

55bb1cc

Extract has property access datapoint.

5aedaf3

Add tests for has property access datapoint.

c830bea

Extract get key events datapoint.

7929c6b

Improve import name.

79f4954

Fix is_shared_datapoint_request logic.

d38443a

abdelmalekkkkk force-pushed the enhancement/11092-refactor-reporting-key-events-datapoints branch 2 times, most recently from 5168e7c to 1a10f88 Compare April 7, 2026 11:42

Address review feedback.

5ab1bf9

abdelmalekkkkk force-pushed the enhancement/11092-refactor-reporting-key-events-datapoints branch from 1a10f88 to 5ab1bf9 Compare April 7, 2026 11:53

Conversation

abdelmalekkkkk commented Mar 26, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Relevant technical choices

PR Author Checklist

Code Reviewer Checklist

Merge Reviewer Checklist

Uh oh!

github-actions bot commented Mar 26, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hussain-t left a comment

Choose a reason for hiding this comment

Uh oh!

abdelmalekkkkk commented Apr 6, 2026

Uh oh!

hussain-t left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

abdelmalekkkkk commented Apr 7, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

abdelmalekkkkk commented Mar 26, 2026 •

edited

Loading

github-actions bot commented Mar 26, 2026 •

edited

Loading